-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: isolate state between duplicate VSCode workspaces #7699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add session-specific keys to ContextProxy to prevent state sharing - Use vscode.env.sessionId to create unique state keys per window - Maintain shared keys for API configurations that should persist - Handle cases where sessionId is not available (e.g., in tests) Fixes #7698
| const oldNestedSettings = this.originalContext.globalState.get<any>("openRouterImageGenerationSettings") | ||
| // Check if there's an old nested structure (use session-specific key) | ||
| const sessionKey = this.getSessionKey("openRouterImageGenerationSettings") | ||
| const oldNestedSettings = this.originalContext.globalState.get<any>(sessionKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In migrateImageGenerationSettings, consider checking both the session-prefixed key and the legacy plain key to ensure smooth migration for users updating from earlier versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
Overall Assessment
The implementation correctly addresses issue #7698 by using vscode.env.sessionId to create session-specific state keys. This ensures proper isolation between duplicate VSCode workspaces.
Additional Suggestions (not inline):
-
Singleton pattern consideration: The
getInstance()method might face race conditions if two VSCode windows initialize simultaneously. Consider if additional synchronization is needed. -
Test coverage: Consider adding edge case tests for concurrent access from multiple windows and migration with pre-existing non-session-prefixed data.
| private getSessionKey(key: string): string { | ||
| // For certain keys that should be shared across sessions (like API configs), | ||
| // we don't add the session prefix | ||
| const sharedKeys = ["listApiConfigMeta", "currentApiConfigName", "apiProvider"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this list to a constant like SHARED_STATE_KEYS since it's duplicated in the test file. This would make maintenance easier and prevent drift between implementation and tests.
Also, could we add a comment explaining why these specific keys need to be shared across sessions? This would help future maintainers understand the design decision.
| // Check if there's an old nested structure | ||
| const oldNestedSettings = this.originalContext.globalState.get<any>("openRouterImageGenerationSettings") | ||
| // Check if there's an old nested structure (use session-specific key) | ||
| const sessionKey = this.getSessionKey("openRouterImageGenerationSettings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration edge case: This checks for session-specific keys, but if settings existed before this PR, they wouldn't have session prefixes. Should we check both sessionKey and the original key to handle pre-existing data?
| const sessionKey = this.getSessionKey("openRouterImageGenerationSettings") | |
| // Check if there's an old nested structure (check both session-specific and non-session keys) | |
| const sessionKey = this.getSessionKey("openRouterImageGenerationSettings") | |
| const oldNestedSettings = this.originalContext.globalState.get<any>(sessionKey) || | |
| this.originalContext.globalState.get<any>("openRouterImageGenerationSettings") |
|
|
||
| // When sessionId is available, session-specific keys are used for non-shared keys | ||
| // In test environment with mocked sessionId, check for session-specific keys | ||
| const sharedKeys = ["listApiConfigMeta", "currentApiConfigName", "apiProvider"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test coverage! The tests properly verify the session-specific key behavior.
|
Issue needs scoping |
Summary
This PR fixes issue #7698 where opening multiple VSCode windows with the same workspace (using "Duplicate Workspace") causes Roo Code instances to share the same state, leading to context being mirrored/overwritten between windows.
Problem
When users open duplicate VSCode workspaces, both Roo Code instances were using the same global state keys, causing:
Solution
The fix uses VSCode's
vscode.env.sessionIdto create session-specific state keys, ensuring each window maintains its own independent state.Key Changes:
ContextProxyclassTesting
Impact
This change ensures that users can:
Fixes #7698
Important
Fixes shared state issue in duplicate VSCode workspaces by using session-specific keys in
ContextProxy.vscode.env.sessionIdinContextProxyto create session-specific state keys, isolating state per VSCode window.apiProvider) across sessions.sessionIdis unavailable.ContextProxymethods likeinitialize(),getSessionKey(),getGlobalState(), andupdateGlobalState()to handle session-specific keys.migrateImageGenerationSettings()to use session-specific keys.ContextProxy.spec.tsto mocksessionIdand verify session-specific key usage.getGlobalState()andupdateGlobalState().This description was created by
for 02e8716. You can customize this summary. It will automatically update as commits are pushed.